feat(rdb save): add blob compression on snapshot #505

adiholden · 2022-11-21T12:44:27Z

Signed-off-by: adi_holden adi@dragonflydb.io

romange · 2022-11-21T12:54:28Z

src/server/rdb_save.cc

@@ -773,13 +778,16 @@ class RdbSaver::Impl {
  RdbSerializer meta_serializer_;
  SliceSnapshot::RecordChannel channel_;
  std::optional<AlignedBuffer> aligned_buf_;
+  bool enhanced_compression_;


please add comment saying what it means.

romange · 2022-11-21T13:01:15Z

src/server/snapshot.h

@@ -19,6 +22,22 @@ struct Entry;

 class RdbSerializer;

+class ZstdCompress {
+ public:
+  ZstdCompress() {


maybe add deleted constructor and operator= to prevent copying by chance.

romange · 2022-11-21T13:02:16Z

src/server/snapshot.h

  size_t channel_bytes_ = 0;
  size_t serialized_ = 0, skipped_ = 0, side_saved_ = 0, savecb_calls_ = 0;
  uint64_t rec_id_ = 0;
  uint32_t num_records_in_blob_ = 0;
-
+  bool enhanced_compression_ = false;
+  ZstdCompress compressor_;


make it unique_ptr<ZstdCompress> to avoid allocating the context in case compression is false.

romange · 2022-11-21T17:06:40Z

src/server/snapshot.cc

+  size_t compressed_size =
+      ZSTD_compressCCtx(cctx_, compr_buf_.data(), compr_buf_.capacity(), str.data(), str.size(), 1);
+  if (compressed_size > str.size() * 0.85) {
+    ++comperssion_no_effective_;


comperssion -> compression

romange · 2022-11-21T17:07:43Z

src/server/snapshot.cc

+void SliceSnapshot::PushFileToChannel(io::StringFile* sfile, DbIndex db_index, unsigned num_records,
+                                      bool should_compress) {
+  string string_to_push = std::move(sfile->val);
+  if (should_compress && string_to_push.size() > 4096) {


please introduce a named variable and maybe reduce its value to 256.
also please check if compression makes sense at all.

romange · 2022-11-21T17:11:14Z

src/server/snapshot.h

@@ -19,6 +22,22 @@ struct Entry;

 class RdbSerializer;

+class ZstdCompress {


nit: i would move this class definition into cc and just forward declare it here.
why - to avoid bringing "zstd.h" includes to all the cc files that include this header since it's really an implementation detail. Also see my comment below about unique_ptr

romange · 2022-11-21T17:13:50Z

src/server/snapshot.cc

+    compr_buf_.reserve(buf_size);
+  }
+  size_t compressed_size =
+      ZSTD_compressCCtx(cctx_, compr_buf_.data(), compr_buf_.capacity(), str.data(), str.size(), 1);


please introduce a flag snapshot_compression_level - it will be convenient for playing with compression.

src/server/rdb_load.cc

romange

Great work. Are you going to send test code in a separate pr?

romange · 2022-11-24T16:25:15Z

src/server/rdb_load.cc

+  // Push uncompressed blob to its membuf
+  uncompressed_mem_buf_.Reserve(uncompressed_blob.size());
+  IoBuf::Bytes dest = uncompressed_mem_buf_.AppendBuffer();
+  CHECK_GE(dest.size(), uncompressed_blob.size());


please avoid to check failing as a result of bad data. please use error_code for this.

Maybe my comment is wrong here. You can not check-fail as a result of bad data, you verify the invariant, right?

romange · 2022-11-24T16:27:13Z

src/server/rdb_load.cc

+  uncompressed_mem_buf_.Reserve(uncompressed_blob.size());
+  IoBuf::Bytes dest = uncompressed_mem_buf_.AppendBuffer();
+  CHECK_GE(dest.size(), uncompressed_blob.size());
+  memcpy(dest.data(), uncompressed_blob.data(), uncompressed_blob.size());


there is some inefficiency here (double write to memory). maybe add todo: to adjust interfaces so that we avoid redundant memory copy.

I see, I am fixing this now

romange · 2022-11-24T16:33:20Z

src/server/rdb_save.cc

@@ -737,7 +743,7 @@ class RdbSaver::Impl {
 public:
  // We pass K=sz to say how many producers are pushing data in order to maintain
  // correct closing semantics - channel is closing when K producers marked it as closed.
-  Impl(bool align_writes, unsigned producers_len, io::Sink* sink);
+  Impl(bool align_writes, unsigned producers_len, bool multi_entires_compression, io::Sink* sink);


multi_entires_compression -> multi_entries_compression

src/server/rdb_save.h

romange · 2022-11-24T16:40:01Z

src/server/snapshot.h

  size_t channel_bytes_ = 0;
  size_t serialized_ = 0, skipped_ = 0, side_saved_ = 0, savecb_calls_ = 0;
  uint64_t rec_id_ = 0;
  uint32_t num_records_in_blob_ = 0;
-
+  bool multi_entires_compression_ = false;


multi_entries_compression_

romange · 2022-11-24T16:41:14Z

src/server/snapshot.cc

+    }
+    auto comp_res = zstd_serializer_->Compress(string_to_push);
+    if (comp_res.first) {
+      string_to_push = comp_res.second;


string_to_push.swap(comp_res.second);

romange · 2022-11-24T16:42:11Z

src/server/rdb_save.cc

+  // Compress the string with end blob opcode
+  string to_compress(str.data(), str.size());
+  opcode = RDB_OPCODE_COMPRESSED_BLOB_END;
+  to_compress.append(reinterpret_cast<const char*>(&opcode), 1);


to_compress.push_back(opcode);

src/server/rdb_load.cc

romange · 2022-11-24T17:35:24Z

src/server/rdb_save.cc

 #include "base/logging.h"
 #include "server/engine_shard_set.h"
 #include "server/error.h"
 #include "server/rdb_extensions.h"
 #include "server/snapshot.h"
 #include "util/fibers/simple_channel.h"

+ABSL_FLAG(bool, save_enhanced_compression, true, "DF save compressed blobs of multiple entries");


there is a bug when you run dragonfly with save_enhanced_compression=false it does not compress strings with lzf as it did beforehand.

Owww not only that but I do double compression lzf and zstd when I have the flag=true

romange · 2022-11-24T17:39:23Z

Please see the profile below.
I run "debug populate 50000000 key 550" and then "save df" when running dragonfly
with "--save_enhanced_compression=true"

romange · 2022-11-27T17:31:36Z

src/server/rdb_save.cc

      producer_count = 0;
+      if (compression_mode == 2) {


Lets keep it as RDB (no need for multi-entry here).

romange · 2022-11-27T17:33:51Z

src/server/rdb_save.cc

+  }
+
+  // Compress the string
+  string_view compresse_res = impl_->Compress(str);


compresse_res -> compressed_res

romange · 2022-11-27T17:34:21Z

src/server/rdb_save.cc

+  // Write encoded compressed string len and than the compressed string
+  serialized_compressed_blob.append(reinterpret_cast<const char*>(buf), enclen);
+  serialized_compressed_blob.append(compresse_res);
+  return std::make_pair(true, serialized_compressed_blob);


std::move(serialized_compressed_blob)

romange · 2022-11-27T17:35:02Z

src/server/rdb_save.h

+  ~ZstdCompressSerializer();
+
+  // Returns a pair consisting of an bool denoting whether the string was compressed
+  // and a string the result of compression. If givven string was not compressed returned


givven -> given

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden requested a review from romange November 21, 2022 12:44

romange reviewed Nov 21, 2022

View reviewed changes

adiholden force-pushed the compress_on_snapshot branch 2 times, most recently from 5b0acf1 to c76a42b Compare November 24, 2022 10:34

romange reviewed Nov 24, 2022

View reviewed changes

src/server/rdb_load.cc Show resolved Hide resolved

romange reviewed Nov 24, 2022

View reviewed changes

adiholden force-pushed the compress_on_snapshot branch 2 times, most recently from 77f0817 to a4b69b4 Compare November 27, 2022 13:45

romange reviewed Nov 27, 2022

View reviewed changes

adiholden force-pushed the compress_on_snapshot branch from a4b69b4 to b52c0f0 Compare November 28, 2022 09:55

adiholden requested a review from romange November 28, 2022 09:58

romange previously approved these changes Nov 28, 2022

View reviewed changes

adiholden dismissed romange’s stale review via fe6bba2 November 28, 2022 13:37

adiholden force-pushed the compress_on_snapshot branch from b52c0f0 to fe6bba2 Compare November 28, 2022 13:37

romange previously approved these changes Nov 28, 2022

View reviewed changes

feat(rdb save): add blob compression on snapshot

9a7cc25

Signed-off-by: adi_holden <adi@dragonflydb.io>

adiholden dismissed romange’s stale review via 9a7cc25 November 29, 2022 08:54

adiholden force-pushed the compress_on_snapshot branch from fe6bba2 to 9a7cc25 Compare November 29, 2022 08:54

romange approved these changes Nov 29, 2022

View reviewed changes

adiholden merged commit 685b441 into main Nov 29, 2022

romange deleted the compress_on_snapshot branch November 29, 2022 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rdb save): add blob compression on snapshot #505

feat(rdb save): add blob compression on snapshot #505

adiholden commented Nov 21, 2022

romange Nov 21, 2022

romange Nov 21, 2022

romange Nov 21, 2022

romange Nov 21, 2022

romange Nov 21, 2022

romange Nov 21, 2022

romange Nov 21, 2022

romange left a comment

romange Nov 24, 2022

romange Nov 27, 2022

romange Nov 24, 2022 •

edited

Loading

adiholden Nov 27, 2022

romange Nov 24, 2022

romange Nov 24, 2022

romange Nov 24, 2022

romange Nov 24, 2022

romange Nov 24, 2022

adiholden Nov 27, 2022

romange commented Nov 24, 2022

romange Nov 27, 2022

romange Nov 27, 2022

romange Nov 27, 2022

romange Nov 27, 2022

		@@ -19,6 +22,22 @@ struct Entry;

		class RdbSerializer;

		class ZstdCompress {

feat(rdb save): add blob compression on snapshot #505

feat(rdb save): add blob compression on snapshot #505

Conversation

adiholden commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Nov 24, 2022 •

edited

Loading